stdlib: export opaque ets:continuation/0 - #11072
Conversation
CT Test Results 2 files 100 suites 1h 1m 26s ⏱️ Results for commit 3021f38. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
|
@sverker 👋🏽 following up here 🙂 |
sverker
added
enhancement
testing
| -opaque continuation() :: '$end_of_table' | ||
| | {table(),integer(),integer(),compiled_match_spec(),list(),integer()} | ||
| | {table(),_,_,integer(),compiled_match_spec(),list(),integer(),integer()}. |
There was a problem hiding this comment.
Even though type continuation() has been documented as "opaque" in text, there might be users that match on '$end_of_table', and I think dialyzer will start complaining about that if continuation() becomes opaque for real.
Another variant could be
-type continuation() :: '$end_of_table' | continuation_tuple().
-opaque continuation_tuple() :: {table(),...} | {table(),...}.
But I don't know if that would help, be the same, or even make it worse.
There was a problem hiding this comment.
If there's a type A defined as a type union B | C, it's not a good idea to make B or C opaque (if they weren't previously). It would imply we can distinguish B and C despite at least one of them being opaque, which is a violation of opacity.
I wouldn't suggest making continuation_tuple() opaque. If continuation() is part of a type union for another type, I wouldn't suggest making it opaque either.
There was a problem hiding this comment.
So, should continuation/0 be exported just as a type? In that case it would leak the whole big tuple shapes, perhaps we want to make it
| -opaque continuation() :: '$end_of_table' | |
| | {table(),integer(),integer(),compiled_match_spec(),list(),integer()} | |
| | {table(),_,_,integer(),compiled_match_spec(),list(),integer(),integer()}. | |
| -type continuation() :: '$end_of_table' | tuple(). |
And entirely hide how it looks? The disadvantage would be that the module would lose internal type checking. What's the best option?
There was a problem hiding this comment.
I think I want continuation() to be exported as not opaque. Even though the documentation text says it is "opaque", it can be returned as '$end_of_table' and I think I want the user to be able to match on '$end_of_table'. The alternative for the user is to do another ets:match/select call with Continuation as '$end_of_table' just to get back a '$end_of_table' that can be matched upon.
In other words, continuation() is in practice a semi-opaque type and I want it to keep being that. The type system does not support semi-opaqueness, so the next best thing is a non-opaque type with some explaining text.
There was a problem hiding this comment.
I think it's better to keep the two tuples as it is and mention in text not to match on them. A tuple() will just make the type checking worse, both internal and for the user.
There was a problem hiding this comment.
Added a not to reflect that 👍🏽
Expose continuation/0 as an exported semi-opaque type so callers can annotate state passed between select/match steps without using term().
9487c5d to
3021f38
Compare
4 participants
Expose
ets:continuation/0as an exported opaque type so callers can annotate state passed between select/match steps without resorting toterm/0.Publicly exported functions have
ets:continuation/0as their exported type so this type is kinda public already.